Attachment api #613

p-avital · 2023-12-05T15:29:05Z

No description provided.

Mallets · 2023-12-05T15:45:34Z

zenoh/src/publication.rs

+    }
+
+    #[zenoh_macros::unstable]
+    pub fn attachments_mut(&mut self) -> &mut Option<Attachments> {


attachments returns Option<&Attachments>, attachments_mut should return Option<&mut Attachments>.

With the mutable accessor, I wanted to give the user the opportunity to straight up remove the attachments, so a reference to the whole option is needed

Mallets · 2023-12-05T15:47:19Z

zenoh/src/query.rs

+    }
+
+    #[zenoh_macros::unstable]
+    pub fn attachments_mut(&mut self) -> &mut Option<Attachments> {


It should return Option<&mut Attachments>.

same as above

Mallets · 2023-12-05T15:47:58Z

zenoh/src/queryable.rs

+    }
+
+    #[zenoh_macros::unstable]
+    pub fn attachments_mut(&mut self) -> Option<&mut Option<Attachments>> {


Why double option?

same as above + getting the attachments on replies is fallible since error replies don't have attachments

Mallets · 2023-12-05T16:54:04Z

zenoh/tests/attachments.rs

+        ); 10];
+        for (j, backer) in backer.iter_mut().enumerate() {
+            *backer = ((i * 10 + j).to_le_bytes(), (i * 10 + j).to_be_bytes())
+        }


I understand that the Iterator pattern being used here allows to avoid some extra copy. However, API-wise is very verbose and not-so elegant. Before going any further, I'd like to see a benchmark (e.g. pub/sub throughput) using the assert.insert(...) and the Iterator approach. I.e.: I'd like to understand the tradeoff between API usability and performance impact.

sashacmc

It would be nice to add usage examples, preferably containing both approaches through an iterator and through a map.

sashacmc · 2023-12-06T18:04:18Z

commons/zenoh-buffers/src/zbuf.rs

+        self.slices.drain(drain_start..drain_end);
+    }
+    fn insert(&mut self, mut at: usize, slice: &[u8]) {
+        if slice.is_empty() {


It would be nice to check if at less than buffer size

indeed, but computing len here is a bit expensive... that check can be done by checking for slice_index == usize::MAX a bit later, adding to the todo list

sashacmc · 2023-12-06T18:05:59Z

zenoh/src/sample.rs

+        pub fn iter(&self) -> AttachmentsIterator {
+            self.into_iter()
+        }
+        fn _get(&self, key: &[u8]) -> Option<ZSlice> {


Why we need _get, if it used only in the get? (the same for the insert)
Is it some rust syntax sugar?

This is for code size optimization: it's a rather common pattern when you want to provide a generic input that's immediately converted to a given type, as that conversion doesn't affect the rest of the function's behaviour.

Supposedly, an optimizer could do that for us, but this helps it to do that job by instead considering if it's worth inlining the common section or not, instead of looking for code sections that could be factorized

sashacmc · 2023-12-07T12:45:33Z

More thoughts.
Based on the name of the branch and for general reasons (we have en attachment which has map of key value pairs), I thought it would be correct to name the feature as an attachment not attachments and in my PR I already did it.
But there still some inconsistency between attachment and attachments.
I think we need to agree on a common name.
@Mallets

p-avital · 2023-12-07T13:07:11Z

More thoughts. Based on the name of the branch and for general reasons (we have en attachment which has map of key value pairs), I thought it would be correct to name the feature as an attachment not attachments and in my PR I already did it. But there still some inconsistency between attachment not attachments. I think we need to agree on a common name. @Mallets

@Mallets agrees with you, I named the type Attachments because I view it as a collection of attachments, each attachment being named (so essentially, I call each value in the key-value pairs an attachment). I'm fine with the plural form being eliminated for consistency.

sashacmc · 2023-12-07T13:28:15Z

@Mallets agrees with you, I named the type Attachments because I view it as a collection of attachments, each attachment being named (so essentially, I call each value in the key-value pairs an attachment). I'm fine with the plural form being eliminated for consistency.

but what confuses me is that in rust we will have with_attachments/attachments methods but in the same time for the same functionality in C/C++ we will have as_attachment / set_attachment / attachment etc.

p-avital · 2023-12-07T14:06:56Z

but what confuses me is that in rust we will have with_attachments/attachments methods but in the same time for the same functionality in C/C++ we will have as_attachment / set_attachment / attachment etc.

In Rust, it's common practice to use with_X(self, x) -> Self in builder patterns; set_X(&mut self, x: X)-> Option<X> being used for setters. Adding a setter could be done, but this role is mostly fulfilled by attachments_mut providing mutable access.

…t trust it anyway

Mallets · 2023-12-13T16:07:48Z

examples/examples/z_pub_thr.rs

@@ -40,10 +53,28 @@ fn main() {
    let mut count: usize = 0;
    let mut start = std::time::Instant::now();
    loop {
-        publisher.put(data.clone()).res().unwrap();
+        let attachments = (args.attachments_number != 0).then(|| {


I suggest to remove the attachments from the z_pub_thr and to have it only on the z_pub/z_sub.

z_sub example should also show how to use the attachment.

@Mallets in zenoh-c I decided to move attachment usage to separate sample to don't overload main simple z_pub/z_sub samples.
Maybe we should do the same here too?

I'm fine to move it to separate examples.

Mallets · 2023-12-13T16:13:01Z

zenoh/src/publication.rs

@@ -117,6 +121,22 @@ impl PutBuilder<'_, '_> {
        self.kind = kind;
        self
    }
+
+    #[zenoh_macros::unstable]
+    pub fn attachment(&self) -> Option<&Attachment> {


Looking at this more closely. What's the interest of attachment and attachment_mut in the PutBuilder? Shouldn't with_attachment be enough? It's a builder, so I would expect to be additive for the attachment...

Agreed for builders

Mallets · 2023-12-13T16:13:41Z

zenoh/src/publication.rs

+
+impl<'a> Publication<'a> {
+    #[zenoh_macros::unstable]
+    pub fn attachment(&self) -> Option<&Attachment> {


Similar comment as for the PutBuilder. What's the interest of attachment and attachment_mut in the PutBuilder? Shouldn't with_attachment be enough? It's a builder, so I would expect to be additive for the attachment...

Mallets · 2023-12-13T16:15:50Z

zenoh/src/query.rs

@@ -294,6 +306,22 @@ impl<'a, 'b, Handler> GetBuilder<'a, 'b, Handler> {
        self
    }

+    #[zenoh_macros::unstable]
+    pub fn attachment(&self) -> Option<&Attachment> {


Similar comment as per other builders. What's the interest of attachment and attachment_mut in the PutBuilder? Shouldn't with_attachment be enough? It's a builder, so I would expect to be additive for the attachment...

Mallets · 2023-12-13T16:16:09Z

zenoh/src/queryable.rs

@@ -150,6 +160,36 @@ pub struct ReplyBuilder<'a> {
    result: Result<Sample, Value>,
 }

+impl<'a> ReplyBuilder<'a> {
+    #[zenoh_macros::unstable]
+    pub fn attachment(&self) -> Option<&Attachment> {


Similar comment as in the other builders... What's the interest of attachment and attachment_mut in the PutBuilder? Shouldn't with_attachment be enough? It's a builder, so I would expect to be additive for the attachment...

Agreed, and this spot is especially painful

Pierre Avital added 2 commits December 5, 2023 16:19

Add support for attachments

0359739

Merge branch 'attachment' into attachment-api

e10eff2

p-avital requested a review from Mallets December 5, 2023 15:29

Mallets reviewed Dec 5, 2023

View reviewed changes

sashacmc reviewed Dec 6, 2023

View reviewed changes

Pierre Avital added 2 commits December 7, 2023 15:39

fix no_std, remove plural

3e9eb0a

Merge branch 'master' into attachment-api

4f7b886

This was referenced Dec 7, 2023

Attachment support implementation eclipse-zenoh/zenoh-c#203

Merged

Attachment support implementation eclipse-zenoh/zenoh-cpp#86

Merged

Pierre Avital added 4 commits December 8, 2023 09:33

add attachment support in throughput tests

091c84f

stop encoding the number of elements at in Attachments, since we can'…

2371e83

…t trust it anyway

provide AttachmentBuilder to have faster Attachment::insert API

3e96a0a

Merge branch 'attachment' into attachment-api

62633bf

Mallets reviewed Dec 13, 2023

View reviewed changes

remove accessors in builders, adapt examples

dd35540

Mallets approved these changes Dec 13, 2023

View reviewed changes

Mallets merged commit 678cc57 into attachment Dec 13, 2023
12 of 15 checks passed

Mallets deleted the attachment-api branch December 13, 2023 17:19

DariusIMP mentioned this pull request Jan 25, 2024

User attachment implementation eclipse-zenoh/zenoh-kotlin#30

Closed

DariusIMP mentioned this pull request Jan 26, 2024

User attachment implementation eclipse-zenoh/zenoh-java#29

Closed

milyin mentioned this pull request Jan 28, 2024

User attachment api #689

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attachment api #613

Attachment api #613

p-avital commented Dec 5, 2023

Mallets Dec 5, 2023

p-avital Dec 7, 2023

Mallets Dec 5, 2023

p-avital Dec 7, 2023

Mallets Dec 5, 2023

p-avital Dec 7, 2023

Mallets Dec 5, 2023

sashacmc left a comment

sashacmc Dec 6, 2023

p-avital Dec 7, 2023

sashacmc Dec 6, 2023

p-avital Dec 7, 2023 •

edited

Loading

sashacmc commented Dec 7, 2023 •

edited

Loading

p-avital commented Dec 7, 2023

sashacmc commented Dec 7, 2023

p-avital commented Dec 7, 2023

Mallets Dec 13, 2023

Mallets Dec 13, 2023

sashacmc Dec 13, 2023 •

edited

Loading

Mallets Dec 13, 2023

Mallets Dec 13, 2023

p-avital Dec 13, 2023

Mallets Dec 13, 2023

Mallets Dec 13, 2023

Mallets Dec 13, 2023

p-avital Dec 13, 2023

Attachment api #613

Attachment api #613

Conversation

p-avital commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashacmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-avital Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

sashacmc commented Dec 7, 2023 • edited Loading

p-avital commented Dec 7, 2023

sashacmc commented Dec 7, 2023

p-avital commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashacmc Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-avital Dec 7, 2023 •

edited

Loading

sashacmc commented Dec 7, 2023 •

edited

Loading

sashacmc Dec 13, 2023 •

edited

Loading